Fix --warn-unreachable fall-through from ALWAYS_TRUE ifs#18539
Fix --warn-unreachable fall-through from ALWAYS_TRUE ifs#18539ammaraskar wants to merge 8 commits intopython:masterfrom
Conversation
Currently blocks marked as unreachable in `reachability.py` through semanal_pass1 are ignored by the `--warn-unreachable` flag through a `block.is_unreachable` check. Namely, reachability marks the if bodies and else-body as unreachable if it is not reachable on the current platform or Python version. This works for if-statements that have an else branch but if the if-statement has a short-circuiting early return or raise on a particular platform, then `reachability.py` places an empty else block and marks it unreachable but has no effect on the fall-through code. When `checker.py` correctly determines that the if-statement is always True and has a return inside it, it correctly marks any code after the if-statement as unreachable. This fix makes it so the code after if-statements that are known to be always true is marked reachable to surpress spurious `--warn-unreachable` warnings.
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
|
From the mypy_primer checks, I guess marking the subsequent code as def _get_win_folder_from_registry(csidl_name: str) -> str:
if sys.platform != "win32":
raise RuntimeError("Method can only be called on Windows.")
import winreg as _winreg
...
_winreg.HKEY_CURRENT_USER,Gonna switch it to def foo() -> int:
if sys.platform != 'fictional':
return 1
return 0
return 0 + 1 # <-- Does not get marked unreachable |
This comment has been minimized.
This comment has been minimized.
|
I'm not convinced about this approach because it feels excessively ad-hoc (surely I should be able to have a The above probably doesn't really make sense but here's the relevant diff (I could make a PR if you want): diff
diff --git a/mypy/binder.py b/mypy/binder.py
index 3d833153d..da31d0e2f 100644
--- a/mypy/binder.py
+++ b/mypy/binder.py
@@ -1,5 +1,5 @@
from __future__ import annotations
-
+import enum
from collections import defaultdict
from collections.abc import Iterator
from contextlib import contextmanager
@@ -35,6 +35,9 @@ class CurrentType(NamedTuple):
type: Type
from_assignment: bool
+class UnreachableType(enum.Enum):
+ BINDER_UNREACHABLE = enum.auto()
+ SEMANAL_UNREACHABLE = enum.auto()
class Frame:
"""A Frame represents a specific point in the execution of a program.
@@ -51,7 +54,7 @@ class Frame:
def __init__(self, id: int, conditional_frame: bool = False) -> None:
self.id = id
self.types: dict[Key, CurrentType] = {}
- self.unreachable = False
+ self.unreachable: UnreachableType | None = None
self.conditional_frame = conditional_frame
self.suppress_unreachable_warnings = False
@@ -161,8 +164,8 @@ class ConditionalTypeBinder:
self._add_dependencies(key)
self._put(key, typ, from_assignment)
- def unreachable(self) -> None:
- self.frames[-1].unreachable = True
+ def unreachable(self, from_semanal: bool = False) -> None:
+ self.frames[-1].unreachable = UnreachableType.SEMANAL_UNREACHABLE if from_semanal else UnreachableType.BINDER_UNREACHABLE
def suppress_unreachable_warnings(self) -> None:
self.frames[-1].suppress_unreachable_warnings = True
@@ -175,13 +178,20 @@ class ConditionalTypeBinder:
return None
return found.type
- def is_unreachable(self) -> bool:
+ def is_unreachable(self) -> UnreachableType | None:
# TODO: Copy the value of unreachable into new frames to avoid
# this traversal on every statement?
- return any(f.unreachable for f in self.frames)
+ result = None
+ for f in self.frames:
+ if f.unreachable and not result:
+ result = f.unreachable
+ elif f.unreachable == UnreachableType.SEMANAL_UNREACHABLE:
+ result = f.unreachable
+
+ return result
def is_unreachable_warning_suppressed(self) -> bool:
- return any(f.suppress_unreachable_warnings for f in self.frames)
+ return any(f.suppress_unreachable_warnings for f in self.frames) or self.is_unreachable() == UnreachableType.SEMANAL_UNREACHABLE
def cleanse(self, expr: Expression) -> None:
"""Remove all references to a Node from the binder."""
@@ -202,6 +212,9 @@ class ConditionalTypeBinder:
If a key is declared as AnyType, only update it if all the
options are the same.
"""
+ if all(f.unreachable for f in frames):
+ self.unreachable(from_semanal=any(f.unreachable == UnreachableType.SEMANAL_UNREACHABLE for f in frames))
+
all_reachable = all(not f.unreachable for f in frames)
frames = [f for f in frames if not f.unreachable]
changed = False
@@ -262,8 +275,6 @@ class ConditionalTypeBinder:
self._put(key, type, from_assignment=True)
changed = True
- self.frames[-1].unreachable = not frames
-
return changed
def pop_frame(self, can_skip: bool, fall_through: int) -> Frame:
@@ -411,7 +422,7 @@ class ConditionalTypeBinder:
for f in self.frames[index + 1 :]:
frame.types.update(f.types)
if f.unreachable:
- frame.unreachable = True
+ frame.unreachable = f.unreachable
self.options_on_return[index].append(frame)
def handle_break(self) -> None:
diff --git a/mypy/checker.py b/mypy/checker.py
index 3734f3170..0e9b8987c 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -3021,7 +3021,7 @@ class TypeChecker(NodeVisitor[None], CheckerPluginInterface):
# This block was marked as being unreachable during semantic analysis.
# It turns out any blocks marked in this way are *intentionally* marked
# as unreachable -- so we don't display an error.
- self.binder.unreachable()
+ self.binder.unreachable(from_semanal=True)
return
for s in b.body:
if self.binder.is_unreachable():However:
I think this is good behavior. Think of every |
Co-authored-by: A5rocks <git@helvetica.moe>
|
I agree, your approach makes way more sense. It is actually what I was initially trying to do but I wasn't sure how the frame ancestor accessibility stuff worked in I've pushed your changes in and added you as a Co-Author, please let me know if that's fine or I can close this and you can make your own PR. |
What you've done is totally fine. |
This comment has been minimized.
This comment has been minimized.
|
I assume the reason why the unused type ignore doesn't get a mention is because the code doesn't get checked because it's unreachable? I wonder why it worked before. |
|
Also, this also fixes:
And the prior aiohttp change in mypy primer didn't actually make sense it seems. Here's the code from aiohttp where it was saying the if ssl is not None:
SSL_ALLOWED_TYPES = (ssl.SSLContext, bool, Fingerprint)
else: # pragma: no cover
SSL_ALLOWED_TYPES = (bool,) # type: ignore[unreachable]so it's good that it's gone. |
|
mypy_primer results look good, we've eliminated false positives in Edit:
Thanks, I went through each of those and made sure it works. Added fixes in the PR description so they get auto-closed. I don't think they need explicit unit tests since they're all seminal level checks which we already cover. |
| from_assignment: bool | ||
|
|
||
|
|
||
| class UnreachableType(enum.Enum): |
There was a problem hiding this comment.
As part of #18707 I've been thinking about related concepts, and I think it's possible to do this without an enum actually: simply mark skipped blocks as suppressed (currently they're only marked as unreachable) and if all the blocks flowing into a block are unreachable, check if any of them are suppressed -- if so, then the current block is suppressed.
|
@ammaraskar mind fixing the conflicts? (otherwise I would be fine to adopt this PR) |
This comment has been minimized.
This comment has been minimized.
|
I'll try to take a look at the CI issue tonight but if I don't respond in a few days feel free to adopt the PR. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: core (https://github.com/home-assistant/core)
+ homeassistant/helpers/config_validation.py:363: error: Unused "type: ignore" comment [unused-ignore]
rich (https://github.com/Textualize/rich)
- rich/_win32_console.py:16: error: Statement is unreachable [unreachable]
- rich/_windows.py:25: error: Statement is unreachable [unreachable]
- rich/_windows.py:40: error: Statement is unreachable [unreachable]
|
|
The code still self-type checks, only fails in mypyc and not much has changed. I think the CI failure is a bug in mypyc. I don't have the bandwidth to look into it at the moment. |
This is a quick swing at trying to fix #10773
Lots of details in my comment on the original issue and the commit message. This was a quick first approach that I came up with, it's not perfect. Since this doesn't tackle handling early
returns in thesemanal_pass1.py, conditional imports etc caused before the short-circuiting return are not considered.return#14940/cc @A5rocks